-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: respect the silence as defined in user LSP settings #131
base: main
Are you sure you want to change the base?
Conversation
do not show a blocking UI window, instead auto-skipping download with a custom console log message LSP will notify via statusbar that the server failed, so no need for an extra status notification here
lsp_cfg = sublime.load_settings('LSP.sublime-settings') | ||
suppress_error_dialogs = cast(bool, lsp_cfg.get('suppress_error_dialogs') or False) | ||
if suppress_error_dialogs: | ||
log_lines.append(' * Download auto-skipped due to "suppress_error_dialogs" setting') | ||
continue | ||
if not sublime.ok_cancel_dialog( | ||
NO_NODE_FOUND_MESSAGE.format(package_name=package_name), 'Download Node.js'): | ||
log_lines.append(' * Download skipped') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not strictly an error dialog but a dialog asking user to install local node instance. Doesn't seem right to me to tie it to that LSP setting.
There is already a way to prevent this dialog from appearing by setting "nodejs_runtime": ["system"]
setting in lsp_utils settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I did want to add non-error dialog setting for technical cases like this :), but that got rejected.
I'd still like to use the local system in general, just not be block-ui-notified about any issues, so the use case is exactly the same, and I can decide whether to just ignore these messages if it's a temp issue with system node, and I don't need a dupe, or install a local node if I don't plan to install a system one.
Although in this specific case there are no commands to manually install, right? So in addition to suppressing the dialog (which is desired) this prevents any local installation (which isn't desired and would be hard to debug)
Could we add a command to install node locally and notify that this command could be used in the message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to use the system node in general then why don't you just set the nodejs_runtime
as I've suggested? You clearly don't want to install a "local" node instance or you would do it once and never care about this dialog anymore.
Your crusade against modal dialog might be going too far in this case. The modality of the dialog is quite useful in this case since it allows user to install node instance and everything works right out of the box then. Otherwise (with just a log message), the LSP would fail to initialize and then the user would have to at least re-open the file after installing local node manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I want to be able to use either, not just the system one. And I don't install the local one when it's a dupe, but in cases when I didn't have a system one that's exactly what I did - installed the local version via this plugin.
LSP is planning to add a user setting allowing for an uniterrupted user experience on various less-than-critical errors.
However, lsp_utils still asks for download in case some local node-resolution failed. This PR checks whether the user has opted into no interruptions in their LSP config and auto-cancels the download and the download prompt with a custom console log message specifying which setting is responsible
LSP will notify via statusbar that the server failed, so no need for an extra status notification in this package
addresses sublimelsp/LSP#2545